Conversation
nikgraf
force-pushed
the
ng/improve-server-error-handling
branch
from
f1cd3ab to
4833f56
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enhances server robustness by adding middleware for request timeouts, centralized error handling, and graceful shutdown logic.
- Introduces a 30s timeout middleware to send 408 responses on slow requests
- Adds global error and 404 handlers for consistent API responses
- Registers process-level handlers for unhandled rejections, uncaught exceptions, and OS signals to cleanly close HTTP and WebSocket servers
Comments suppressed due to low confidence (2)
apps/server/src/index.ts:71
- Add tests to verify the 30s timeout triggers correctly and returns a 408, and to simulate long-running routes to confirm the behavior.
// Request timeout middleware
apps/server/src/index.ts:74
- [nitpick] For consistency with other error responses, consider adding a
messagefield alongsideerror, or align the JSON shape with your global error handler.
res.status(408).json({ error: 'Request timeout' });
| console.error('Unhandled Rejection at:', promise, 'reason:', reason); | ||
| }); | ||
|
|
||
| process.on('uncaughtException', (error) => { |
There was a problem hiding this comment.
In this handler, you close only the HTTP server. To fully shut down, also call webSocketServer.close() before exiting to avoid leaving socket connections open.
|
|
||
| // Global process error handlers | ||
| process.on('unhandledRejection', (reason, promise) => { | ||
| console.error('Unhandled Rejection at:', promise, 'reason:', reason); |
There was a problem hiding this comment.
Uncaught promise rejections can leave the process in an inconsistent state. Consider invoking the same shutdown logic as for uncaughtException or at least logging and exiting after a grace period.
| console.error('Unhandled Rejection at:', promise, 'reason:', reason); | |
| console.error('Unhandled Rejection at:', promise, 'reason:', reason); | |
| // Graceful shutdown | |
| server.close(() => { | |
| console.log('Server closed due to unhandled rejection'); | |
| webSocketServer.close(() => { | |
| console.log('WebSocket server closed'); | |
| process.exit(1); | |
| }); | |
| }); | |
| // Force close after 30 seconds | |
| setTimeout(() => { | |
| console.error('Could not close connections in time, forcefully shutting down'); | |
| process.exit(1); | |
| }, 30000); |
| res.setTimeout(30000, () => { | ||
| res.status(408).json({ error: 'Request timeout' }); | ||
| }); |
There was a problem hiding this comment.
After sending the timeout response, ensure no further handlers write to the response (e.g., by storing a flag or directly ending the response) to avoid double send errors.
| res.setTimeout(30000, () => { | |
| res.status(408).json({ error: 'Request timeout' }); | |
| }); | |
| let responseSent = false; | |
| res.setTimeout(30000, () => { | |
| if (!responseSent) { | |
| responseSent = true; | |
| res.status(408).json({ error: 'Request timeout' }); | |
| } | |
| }); | |
| res.on('finish', () => { | |
| responseSent = true; | |
| }); |
1 participant
No description provided.